Sol review fixes#61
Merged
Merged
Conversation
A throw from a worker chunk previously called std::terminate (unhandled exception in std::thread), and a throw from the thread-0 chunk destroyed joinable threads — either way aborting the whole process, e.g. for negative labels in the distributed block scanners with number_of_threads > 1. Capture the first exception, always join every worker, and rethrow on the calling thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The naive sum_of_squares/count - mean^2 formula suffers catastrophic cancellation for values with a large baseline and small spread (e.g. values [1e8, 1e8+1] gave std 0.0 instead of 0.5). - Distributed: switch the (n, 5) partial-stats layout from [count, sum, sum_of_squares, min, max] to the Welford/Chan representation [count, mean, M2, min, max]; merge via the Chan combine, finalize as variance = M2 / count. - In-core complex features: compute the variance two-pass from the values already stored for the percentiles (exact). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
merge_block_edge_stats copied the entire (E, 5) global accumulator on every call, making the documented per-block merge loop O(number_of_blocks x global_edges). Mutate the caller's accumulator in place instead (and return it, so existing loops keep working): one merge is now O(block edges) regardless of global graph size. The Python wrapper requires a C-contiguous writable float64 accumulator and no longer coerces it (a silent copy would discard the update). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unordered_set added ~8x node/bucket overhead over the raw edge data (about 114 MB extra for one million edges). Canonicalize into a vector and sort + unique instead: same sorted-unique output, peak memory close to one copy of the input. Document that outputs are valid inputs, so callers can bound peak memory with a hierarchical merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- own_begin / own_shape now require integer dtypes (floats such as [0.9, 0] were silently truncated by int()). - Affinity offsets go through operator.index, which rejects floats. - merge_edges rejects floating-dtype edge arrays and negative node ids in signed arrays (an int64 edge [-1, 2] silently wrapped to [2, 2**64 - 1] via the unconditional uint64 cast). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The core statics return the raw _core.UndirectedGraph, so results lacked the subclass convenience wrappers (find_edges on lists, ...) and failed isinstance checks against bic.graph.UndirectedGraph. Add a constructor overload taking (number_of_nodes, uvs, unique) — an __init__ overload constructs the derived Python class, unlike a static — and build the subclass classmethods on it. from_edges also gains the single-pass C++ construction instead of insert_edges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 2D/3D offset-box sweeps in feature_accumulation.hxx and the
owned-box sweeps in the distributed block extraction duplicated the
same clamp + loop logic. Move a single sweep_clipped_box_{2d,3d} with a
per-axis clip box into detail/grid.hxx; the in-core wrappers pass the
thread slab with unclipped remaining axes, the distributed dispatch
folds the owned box and slab into the clips. Inner loops are unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch fixes several issues in the distributed graph implementation uncovered by 5.6-sol, verified and fixed by Fable.